Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix subtitle track selection when reusing media element with mismatched TextTracks #5880

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Fix subtitle track selection when reusing media element with mismatched TextTracks #5880

merged 3 commits into from
Oct 10, 2023

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Oct 5, 2023 •
edited
Loading

This PR will...

Use subtitle track name and language to find and match subtitle/captions TextTrack rather than subtitleTrack index.

Logs a warning when loading a new source with subtitle options that do not completely overlap with the attached media element's existing TextTracks:

Media element contains unused subtitle tracks: {TRACK LABEL CSVs}.
Replace media element for each source to clear TextTracks and captions menu.

Default subtitle track selection is updated to pick first default | forced | autoselect track to more closely match default behavior with Safari HLS playback.

If the user sets hls.audioTrack to a valid index on AUDIO_TRACKS_UPDATED, that selection will not be replaced with the default audio track immediately after (similar to existing initial subtitle track selection logic).

Why is this Pull Request needed?

Lack of removeTextTrack in HTMLMediaElement standard (whatwg/html#1921).

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Resolves #4571
Replaces #5371

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@robwalch robwalch added this to the 1.5.0 milestone Oct 5, 2023
@robwalch
Use subtitle track name and language to find and match subtitle/capti…
4a0b616 
…ons TextTrack rather than subtitleTrack index

Resolves #4571
@robwalch robwalch force-pushed the bugfix/text-track-reuse-and-selection branch from e48894c to 4a0b616 Compare October 5, 2023 01:45
@robwalch robwalch requested review from tjenkinson and removed request for tjenkinson October 5, 2023 01:45
@robwalch
Copy link
Collaborator Author

robwalch commented Oct 5, 2023 •
edited
Loading

Hi @JudithDu,

Let me know how this fix works for you.

Keep in mind that if you are using HTMLMediaElement controls, you really should replace the video element when loading a new source that could have a different list of captions and subtitle options. If you are using your own controls, consider setting renderTextTracksNatively to false and using the API events to list subtitle options along with something like VTT.js to render them.

@JudithDu
Copy link

JudithDu commented Oct 6, 2023

Hi @robwalch,
Thanks for the quick fix! 🙏
I tested it but encoutered a bug related to the function toggleTrackModes
In my manifest, the subtitles are defined like this
#EXT-X-MEDIA:TYPE=SUBTITLES,GROUP-ID="subtitles",ASSOC-LANGUAGE="fr",NAME="Français (autogenerated)" ...
I have a ASSOC-LANGUAGE but no LANGUAGE (those 2 attributes are optionnal)
In the toggleTrackModes function you test track.language === lang which returns false so the subtitles aren't returned.
As the name is the only attribute required here, couldn't it be enough to test track.label === name?

@robwalch
Fix regression in dev where AUDIO_TRACKS_UPDATED is re-dispatched wit…
6562c19 
…h zero tracks

Allow selection of audioTrack on AUDIO_TRACKS_UPDATED (overriding default selection - Resolves #5831)
Cleanup selection of subtitleTrack on SUBTITLE_TRACKS_UPDATED (overriding default selection)
@robwalch robwalch force-pushed the bugfix/text-track-reuse-and-selection branch from ab535a0 to 6562c19 Compare October 6, 2023 16:57
@robwalch
Fix TextTrack use with ASSOC-LANGUAGE
230a7d8 
Match the initial default/forced/autoselect choice made when playing HLS in Safari
@robwalch
Copy link
Collaborator Author

robwalch commented Oct 6, 2023

HI @JudithDu,

I have a ASSOC-LANGUAGE but no LANGUAGE (those 2 attributes are optionnal)
In the toggleTrackModes function you test track.language === lang which returns false so the subtitles aren't returned.
As the name is the only attribute required here, couldn't it be enough to test track.label === name?

Ah! Language is optional in HLS and the hls.js track, but not on HTMLMediaElement TextTracks. I just pushed a change that will handle empty TextTrack language, and also makes the name and language comparison case-insensitive.

While I was there I found some inconsistencies in initial track selection and made some updates to more closely match the initial default/forced/autoselect choice made when playing HLS in Safari.

@robwalch robwalch mentioned this pull request Oct 6, 2023
Closed
@JudithDu
Copy link

Hello @robwalch
I just tested and it works perfectly fine on my side 👌
Thank you for your work! 🙏

@robwalch robwalch merged commit 21213d5 into master Oct 10, 2023
@robwalch robwalch deleted the bugfix/text-track-reuse-and-selection branch October 10, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
2 participants
@robwalch @JudithDu